feat: adding hermes to e2e tests. #3408

DimitrisJim · 2023-04-04T10:35:23Z

Description

closes: #3315

Commit Message / Changelog Entry

feat: integrating hermes in our e2e tests

see the guidelines for commit messages. (view raw markdown for examples)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

DimitrisJim · 2023-04-04T10:38:00Z

.github/workflows/e2e-test-workflow-call.yml

@@ -50,7 +50,7 @@ on:
      relayer-type:
        description: "The type of relayer to use"
        required: false
-        default: "rly"
+        default: "hermes"


Temporarily, for checking test results

will we be reverting this before merging?

DimitrisJim · 2023-04-04T11:17:08Z

.github/workflows/e2e-test-workflow-call.yml

        type: string
      relayer-tag:
        description: "The tag to use for the relayer"
        required: false
-        default: "" # the tests themselves will choose a sensible default when unset.
+        default: "1.4.0" # the tests themselves will choose a sensible default when unset.


ditto, temporarily.

also reverting this

DimitrisJim · 2023-04-11T09:49:55Z

Pending upstream merge of the changes in interchaintesting strangelove-ventures/interchaintest#493 and a bump of the cosmos-sdk to 0.47.1 (since interchaintesting bumped that recently!) ref #3459

… finally over.

DimitrisJim · 2023-05-15T09:04:43Z

So, the biggest changes here are basically the need to add the following after the hermes relayer starts:

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

it appears as if some time is always required before hermes is able to see that packets have been relayed after starting, one explanation for this is possibly that its startup time is slower than that for rly.

I think it shouldn't be too hard to add the previous snippet in s.StartRelayer and discriminate based on relayer.

colin-axner

LGTM! Happy to approve, just curious if we can figure out a way to reduce diffs and hopefully decrease the time spent debugging missing wait commands?

colin-axner · 2023-05-16T11:38:01Z

e2e/relayer/relayer.go

-	cosmosRelayerUser       = "100:1000"        // docker run -it --rm --entrypoint echo ghcr.io/cosmos/relayer "$(id -u):$(id -g)"
+	hermesRelayerRepository = "ghcr.io/informalsystems/hermes"
+	hermesRelayerUser       = "1000:1000"
+	rlyRelayerRepository    = "damiannolan/rly" //"ghcr.io/cosmos/relayer"


should this be changed back?

nope, these are used when creating the relayer in newHermesRelayer:

ibc-go/e2e/relayer/relayer.go

Line 61 in 77bc38d

customImageOption := relayer.CustomDockerImage(hermesRelayerRepository, tag, hermesRelayerUser)

sorry I mean't the damiannolan/rly string?

ah, right, that's pending on cosmos/relayer#1102

colin-axner · 2023-05-16T11:38:19Z

e2e/relayer/relayer.go

+	hermesRelayerRepository = "ghcr.io/informalsystems/hermes"
+	hermesRelayerUser       = "1000:1000"
+	rlyRelayerRepository    = "damiannolan/rly" //"ghcr.io/cosmos/relayer"
+	rlyRelayerUser          = "100:1000"        // docker run -it --rm --entrypoint echo ghcr.io/cosmos/relayer "$(id -u):$(id -g)"


I think there's a public function for this now, but can't remember

Here we are

looked into this. Using a custom image (like we currently do) doesn't use the defaults. Can't use the defaults for rly since we're based on Damian's forked image, can't use the default for hermes since interchaintest hasn't updated their registry url (still points to docker rather than ghcr + is on version 1.2.0). I'll probably open a PR for the second case on interchaintest and we can drop both consts for hermes.

cool, once we switch off damian's image (probably after v7.1 release) then we can move to the defaults. Is there an issue to track this work? If not, maybe we can open an issue

If not, maybe we can open an issue

yup! done with #3615

re: hermes image/tag updates done strangelove-ventures/interchaintest#571

e2e/tests/interchain_accounts/base_test.go

seanchen1991 · 2023-06-01T14:21:13Z

Hey guys, just want to remind you all that Hermes v1.5 is out. It might be worth updating to the latest version of Hermes in your tests 🙂

colin-axner

Thank you @DimitrisJim!! 🎉

e2e/testsuite/testsuite.go

DimitrisJim · 2023-06-01T16:18:17Z

thanks for the heads up @seanchen1991 I'll bump it now since we hardcode it atm in our tests until we can update to the latest interchaintest version (which should also be bumped!)

DimitrisJim · 2023-06-06T09:28:20Z

Think this should be gtg (will rollback temporary changes to workflow files when necessary). Waiting for ten blocks does suffice for hermes in this case.

Re: bumping to 1.5. Doing this results in the conf for hermes not being found. When I bumped into this case previously, Cian helped out by mentioning it might be a uid/gid issue, which it was. Tweaking that now doesn't seem to have an effect.

I think its fine if we punt this to a later point. Hopefully, we'll be able to sync up with latest interchaintest (which defines the tag/version for hermes so we don't need to redefine here) for which I'll try to bump the version.

chatton

LGTM!

If we have all tests passing with hermes (or at least all the ones we know should be passing)

I would be happy to merge this once we revert to the go relayer as default and then we can discuss what we want to do re hermes/rly for our PRs/compatibility testing etc in a future discussion1

thanks for all the work on this!

The LOC does not reflect the effort you put in to get this all working 🥇

chatton · 2023-06-07T10:05:02Z

e2e/testsuite/testsuite.go

@@ -128,7 +128,7 @@ func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channel
 			}
 		})
 		// wait for relayer to start.
-		time.Sleep(time.Second * 10)
+		s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB), "failed to wait for blocks")


this is what we will be able to remove in the future right?

Considering this was in place with rly (only in sleep form) maybe we'll always need to give relayers some time to get up and running. Ideally, we should be able to drop this to 5 blocks (bumped it to 10 for hermes since 5 resulted in old familiar errors indicating that it needed more time).

chatton · 2023-06-07T10:05:44Z

.github/workflows/e2e-test-workflow-call.yml

@@ -50,7 +50,7 @@ on:
      relayer-type:
        description: "The type of relayer to use"
        required: false
-        default: "rly"
+        default: "hermes"


will we be reverting this before merging?

chatton · 2023-06-07T10:05:53Z

.github/workflows/e2e-test-workflow-call.yml

        type: string
      relayer-tag:
        description: "The tag to use for the relayer"
        required: false
-        default: "" # the tests themselves will choose a sensible default when unset.
+        default: "1.4.0" # the tests themselves will choose a sensible default when unset.


also reverting this

chatton

Latest changes ACK

If we have all tests passing with hermes (or at least all the ones we know should be passing)

I would be happy to merge this once we revert to the go relayer as default and then we can discuss what we want to do re hermes/rly for our PRs/compatibility testing etc in a future discussion1

thanks for all the work on this!

The LOC does not reflect the effort you put in to get this all working 🥇

DimitrisJim linked an issue Apr 4, 2023 that may be closed by this pull request

Integrate Hermes into our E2E tests #3315

Closed

3 tasks

DimitrisJim commented Apr 4, 2023

View reviewed changes

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch 4 times, most recently from 810d6f4 to f44dd9a Compare April 5, 2023 14:31

DimitrisJim mentioned this pull request Apr 11, 2023

Fix issues with hermes client, channel creation and channel queries. strangelove-ventures/interchaintest#493

Merged

DimitrisJim added 7 commits April 26, 2023 23:27

feat: adding hermes to e2e tests.

67d95cf

Hardcode the tag for testing.

a526e8c

Point interchaintest to fork.

a4475ed

Add delays after interchain account registration.

f747125

Add additional delays to failing tests.

ea35187

Update fork; add a wait in MsgPayPacketFeeSingleSender.

bf9252d

Point back to interchaintest.

3fb3004

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from 2831435 to 3fb3004 Compare April 26, 2023 20:40

DimitrisJim added 2 commits April 26, 2023 23:51

Remove import identifiers for interchaintest when not needed.

efb2244

Manually clean up go.sum's entries for the fork. The fork struggle is…

394e531

… finally over.

DimitrisJim marked this pull request as ready for review April 27, 2023 08:39

DimitrisJim requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei, damiannolan and chatton as code owners April 27, 2023 08:39

DimitrisJim and others added 4 commits April 27, 2023 14:23

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

921ec82

Add additional waiting for blocks before querying for packets.

4e0db76

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

99d74b0

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

2bb1e3e

Add a wait for the packets in additional places.

77bc38d

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from ab3f9b5 to 77bc38d Compare May 15, 2023 08:45

colin-axner reviewed May 16, 2023

View reviewed changes

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from 33f17ed to 77bc38d Compare May 23, 2023 11:36

DimitrisJim and others added 2 commits May 23, 2023 15:25

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

3dad322

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

f387da5

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from e34b306 to 7931429 Compare June 1, 2023 07:35

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

777fced

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from 7931429 to 777fced Compare June 1, 2023 07:49

Move waiting for blocks in the start relayer function.

b56a20e

colin-axner approved these changes Jun 1, 2023

View reviewed changes

e2e/testsuite/testsuite.go Outdated Show resolved Hide resolved

DimitrisJim mentioned this pull request Jun 1, 2023

Use interchaintest hermes tag/image instead of duplicating it in e2e tests #3730

Closed

3 tasks

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from 23bd0e7 to 590046d Compare June 1, 2023 16:52

experiment: remove the sleep, keep waiting for 10 blocks.

07b76fb

DimitrisJim force-pushed the jim/3315-integrate-hermes-into-our-e2e-tests branch from 590046d to 07b76fb Compare June 1, 2023 17:12

DimitrisJim added 3 commits June 1, 2023 20:46

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

0c9afbb

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

7afcd2e

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

2ae97a3

chatton approved these changes Jun 7, 2023

View reviewed changes

DimitrisJim added 2 commits June 7, 2023 13:13

Merge branch 'main' into jim/3315-integrate-hermes-into-our-e2e-tests

e8ea7d4

Revert. Set rly as the default in the workflow file.

c45be2a

DimitrisJim merged commit 0b31671 into main Jun 7, 2023

DimitrisJim deleted the jim/3315-integrate-hermes-into-our-e2e-tests branch June 7, 2023 11:28

DimitrisJim mentioned this pull request Jun 12, 2023

Add a scheduled run for hermes. #3822

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding hermes to e2e tests. #3408

feat: adding hermes to e2e tests. #3408

DimitrisJim commented Apr 4, 2023 •

edited

Loading

DimitrisJim Apr 4, 2023

chatton Jun 7, 2023

DimitrisJim Apr 4, 2023

chatton Jun 7, 2023

DimitrisJim commented Apr 11, 2023 •

edited

Loading

DimitrisJim commented May 15, 2023

colin-axner left a comment

colin-axner May 16, 2023

DimitrisJim May 16, 2023

colin-axner May 17, 2023

DimitrisJim May 17, 2023

colin-axner May 16, 2023

colin-axner May 17, 2023

DimitrisJim May 17, 2023 •

edited

Loading

colin-axner May 17, 2023

DimitrisJim May 18, 2023

DimitrisJim May 18, 2023

seanchen1991 commented Jun 1, 2023

colin-axner left a comment

DimitrisJim commented Jun 1, 2023

DimitrisJim commented Jun 6, 2023 •

edited

Loading

chatton left a comment

chatton Jun 7, 2023

DimitrisJim Jun 7, 2023 •

edited

Loading

chatton Jun 7, 2023

chatton Jun 7, 2023

chatton left a comment •

edited

Loading

feat: adding hermes to e2e tests. #3408

feat: adding hermes to e2e tests. #3408

Conversation

DimitrisJim commented Apr 4, 2023 • edited Loading

Description

Commit Message / Changelog Entry

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim commented Apr 11, 2023 • edited Loading

DimitrisJim commented May 15, 2023

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim May 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanchen1991 commented Jun 1, 2023

colin-axner left a comment

Choose a reason for hiding this comment

DimitrisJim commented Jun 1, 2023

DimitrisJim commented Jun 6, 2023 • edited Loading

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DimitrisJim Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton left a comment • edited Loading

Choose a reason for hiding this comment

DimitrisJim commented Apr 4, 2023 •

edited

Loading

DimitrisJim commented Apr 11, 2023 •

edited

Loading

DimitrisJim May 17, 2023 •

edited

Loading

DimitrisJim commented Jun 6, 2023 •

edited

Loading

DimitrisJim Jun 7, 2023 •

edited

Loading

chatton left a comment •

edited

Loading